-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix promotion code batch #5383
base: main
Are you sure you want to change the base?
Fix promotion code batch #5383
Conversation
Before this fix, when the Spree::PromotionCode creation failed, the exception was raised and the job fails with error. This commit rescues the creation exception and remove the failed code from the codes list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if we never reach the number of codes needed? The validation might fail consistently and we will end up in an infinite loop, no?
@@ -39,13 +39,15 @@ def generate_random_codes | |||
new_codes = Array.new(max_codes_to_generate) { generate_random_code }.uniq | |||
codes_for_current_batch = get_unique_codes(new_codes) | |||
|
|||
codes_for_current_batch.each do |value| | |||
codes_for_current_batch = codes_for_current_batch.map do |value| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collecting results of large code counts can exceed available memory pretty quickly.
codes_for_current_batch = codes_for_current_batch.map do |value| | ||
Spree::PromotionCode.create!( | ||
value: value, | ||
promotion: promotion, | ||
promotion_code_batch: promotion_code_batch | ||
) | ||
end | ||
rescue ActiveRecord::RecordInvalid | ||
nil | ||
end.compact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To address @tvdeyen's concern, what about something like:
codes_for_current_batch.filter! do |value|
Spree::PromotionCode.create!(
value: value,
promotion: promotion,
promotion_code_batch: promotion_code_batch
)
rescue ActiveRecord::RecordInvalid
false
end
This should maintain the existing performance characteristics.
This was initially proposed on another PR that we are basing this one on; solidusio#5383 Co-authored-by: Jared Norman <[email protected]> Co-authored-by: Nick <[email protected]> Co-authored-by: Benjamin <[email protected]> Co-authored-by: Sofia <[email protected]> Co-authored-by: Senem Soy <[email protected]> Co-authored-by: Andrew Stewart <[email protected]> Co-authored-by: Chris <[email protected]>
Collecting results of large code counts can exceed available memory. This was initially proposed on another PR that we are basing this one on: solidusio#5383 Co-authored-by: Jared Norman <[email protected]> Co-authored-by: Nick <[email protected]> Co-authored-by: Benjamin <[email protected]> Co-authored-by: Sofia <[email protected]> Co-authored-by: Senem Soy <[email protected]> Co-authored-by: Andrew Stewart <[email protected]> Co-authored-by: Chris <[email protected]>
Summary
When a new promotion with 2.5m new promo codes has created, it just generated about 250k promo codes and then stopped.
The PromotionBatch screen this error: Errored: #<ActiveRecord::RecordInvalid: Validation failed: Value has already been taken>
This PR rescues the exception raised by the Spree::PromotionCode creation and jumps to the next code to save, avoiding stopping the process completely.
The PR also adds the ability to restart the PromotionCodeBatch from the latest PromotionCodes created.
Fixes: #5379
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: